Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Adds types to Anchor #537

Closed
wants to merge 17 commits into from
Closed

Conversation

macalinao
Copy link
Contributor

No description provided.

@macalinao
Copy link
Contributor Author

cc @cheaney might be helping with this

@crispheaney
Copy link
Contributor

@armaniferrante I made the escrow example a typescript test. Let me now if you think we need more tests.

Also note that I needed to add a bash script (create-idl-type.sh) to generate the IDL type for the program. This was based on a script @macalinao shared with me. I think we'll need to add something similar to anchor-cli or @project-serum/anchor so that devs don't have to do this themselves.


describe("escrow", () => {
const provider = anchor.Provider.env();
anchor.setProvider(provider);

const program = anchor.workspace.Escrow;
const program = anchor.workspace.Escrow as anchor.Program<EscrowIDL>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm the account namespace doesn't type check. I can do program.account.notAValidType and it compiles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this expected @macalinao or did I miss something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have forgotten to add this in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this by tweaking the AccountNamespace. But for this to work, I also needed to customize the script that produces the idl type such that the Account names are camel case.

I think we'd want to think through the dev flow for producing the IDL type definition if we are to ship this. Maybe we add a script to anchor-cli that produces the IDL types for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this on your radar @armaniferrante

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ready for another review/testing?

@macalinao
Copy link
Contributor Author

To anyone here: I've published a package here: https://github.com/saber-hq/saber-common/tree/master/packages/anchor-contrib

This contains most of these types. It's undocumented but feel free to ping me on Telegram (@imacalinao) if you're using it!

ChewingGlass added a commit to StrataFoundation/strata that referenced this pull request Aug 19, 2021
@ChewingGlass
Copy link
Contributor

@macalinao Great work on this PR! What needs to happen to kick it across the line? I'm ready to help out where needed

@armaniferrante
Copy link
Member

@macalinao Great work on this PR! What needs to happen to kick it across the line? I'm ready to help out where needed

@crispheaney is this WIP still or is it ready to go for another round of testing?

@crispheaney
Copy link
Contributor

crispheaney commented Aug 21, 2021

@macalinao Great work on this PR! What needs to happen to kick it across the line? I'm ready to help out where needed

@crispheaney is this WIP still or is it ready to go for another round of testing?

I fixed the AccountNamespace bug you surfaced, so in that sense it's ready to go for more testing.

Before we merge this in, I think we'd want to define the dev flow for taking their target's IDL and transforming it the IDL type. I made a hacky script that does this.

@ChewingGlass
Copy link
Contributor

Before we merge this in, I think we'd want to define the dev flow for taking their target's IDL and transforming it the IDL type. I made a hacky script that does this.

As someone consuming this, I think the hacky way is fine. Not everyone will want their types in the same place, I wrote our own custom script because we have a lerna setup and I want the generated code in its respective package. I think documenting it well would suffice for now, then automate later.

[K in T[number]["name"]]: TypeDef<T[number] & { name: K }, Defined>;
};

export type IdlTypes<T extends Idl> = TypeDefDictionary<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also useful to add and export:

export type IdlAccounts<T extends Idl> = TypeDefDictionary<
  NonNullable<T["accounts"]>,
  Record<string, never>
>;

@ChewingGlass
Copy link
Contributor

This has a bunch of conflicts now, I'm going to create a separate PR since I did the merge. #795

@ifiokjr
Copy link

ifiokjr commented Oct 6, 2021

@armaniferrante can this be closed?

@Henry-E
Copy link

Henry-E commented Nov 22, 2022

Yeh, this seems like a dead PR. Please use anchor typescript or anchor-client-gen for a static typescript client.

@Henry-E Henry-E closed this Nov 22, 2022
TopDeveloper917 added a commit to TopDeveloper917/strata that referenced this pull request Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants